Add api of registering storage #32

wisererik · 2020-04-30T10:54:34Z

What this PR does / why we need it:

Add api of registering storage, including:

API validation of registering storage
DB implementations of registering storage
DriverManager.register_storage
FakeDrver.register_storage

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

PravinRanjan10 · 2020-05-01T07:51:04Z

dolphin/drivers/manager.py

+            # todo
+            # access_info['password'] = cryptor.encode(
+            #     access_info['password'])
+            db.access_info_create(context, access_info)


any reason to messup the current architecture? I mean any specififc reason to add database creation, access logic to drivermanager?. In this way another thirdparty controller can not call our drivermanager.

it's inevitable that the driver manager need to access the db, such as the driver or driver manager get the access info from db. we can not pass or save access info in memory because it contains the credential data.

I try to move db access to api layer first.

Seems no consistency mechanism here, need to consider the situation that access_info created in db successfully, but storage created failed.

Thanks, I will concern about it.

NajmudheenCT · 2020-05-04T04:19:52Z

dolphin/db/sqlalchemy/api.py

-    this_session.commit()
-    return register_ref
+    if not values.get('storage_id'):
+        values['storage_id'] = uuidutils.generate_uuid()


we want to maintain storage_id same in all tables. 'storage_id" should be the one returned from driver.
if driver is not returning 'storage_id' we should fail the registration in resource_manager itslef

storage_id should not be return by driver, it's a database id. for each creation operaion, uuid will be created if clients (such as api, driver manager, task manager) don't specify it in the model.

we discussed to move this to driver manager_ , if storage_id is not provided by driver, driver manager generates a uuid..

I didn't get your point here, it's a db implementation, the logic flow is if it doesn't contain id, it will generate, should not depend on the driver manager here. Uuid generation is also handled by driver manager

fine.. hope it is ok to have double checking here..

IMHO, this is ok to have double check but not a good practice. Anything related to storage attrib/ID should be handled till driver/driver manager layer and should not be exposed above. If the client doesn't get storage_id, it should throw exception and not try to create one.

no double check here, the code help caller to generate the uuid if the caller doesn't generate.

NajmudheenCT · 2020-05-04T04:25:04Z

dolphin/db/sqlalchemy/api.py



 def storage_create(context, values):
    """Add a storage device from the values dictionary."""
+    if not values.get('id'):
+        values['id'] = uuidutils.generate_uuid()


same here. Let us not generate storage_id internally. this will create conflict between tables. We will use the storage_id returned by driver.

NajmudheenCT · 2020-05-04T04:26:36Z

dolphin/db/sqlalchemy/api.py

-        .filter(Storage.id == storage_id) \
-        .first()
-    return storage_by_id
+    return _storage_get_get(context, storage_id)


is there a reason why we do this logic in "_storage_get_get"

to support multiple pagenation and sorting. These codes are from openstack nova. it may accelerate our development

sorry, it's typo, has updated it.

NajmudheenCT · 2020-05-04T04:36:51Z

dolphin/drivers/manager.py

+        # Check same access info from DB
+        access_info = copy.deepcopy(register_info)
+        vendor, model = access_info.pop('vendor'), access_info.pop('model')
+        db_access_info = db.access_info_get_all(context, sort_keys=['host'],


register_info is there as input this function. Why do we need to contact db and get access_info?
Also contacting db from driver layer is not according to our architecture..
plz consider

same with Pravin's

NajmudheenCT · 2020-05-04T04:39:37Z

dolphin/drivers/manager.py

+            db.access_info_create(context, access_info)
+
+            storage['id'] = storage_id
+            storage = db.storage_create(context, storage)


storage_create has to be from upper layer

NajmudheenCT · 2020-05-04T04:42:25Z

dolphin/api/v1/storages.py

-            raise exception.DolphinException(e)
-        except Exception as e:
-            msg = _('Failed to register device in driver :{0}'.format(e))
+            storage = driver.register_storage(ctxt, body)


Need to have the logic of validating storage object and creating DB entry for access_info and storage here.

it's already there, plz check the header of this function.

NajmudheenCT

Suggest to change the tile of PR as " Refatoring Resgister API and adding driver manger code for registering"

sfzeng · 2020-05-05T03:27:47Z

dolphin/api/schemas/storages.py

+        'host': parameter_types.hostname_or_ip_address,
+        'port': parameter_types.tcp_udp_port,
+        'username': {'type': 'string', 'minLength': 1, 'maxLength': 255},
+        'password': {'type': 'string'},


Any limitation for password? Min length, for example.

yes, has added.

sfzeng · 2020-05-05T03:32:15Z

dolphin/api/v1/storages.py

-
-        return device_info
+            raise e
+        # except Exception as e:


Remove unused codes here ...

sfzeng · 2020-05-05T03:36:40Z

dolphin/db/api.py

+
+def is_orm_value(obj):
+    """Check if object is an ORM field."""
+    return IMPL.is_orm_value(obj)


Need new line at end.

NajmudheenCT · 2020-05-05T06:34:31Z

dolphin/db/sqlalchemy/models.py

+    host = Column(String(255))
+    port = Column(String(255))
+    username = Column(String(255))
+    password = Column(String(255))


Acess_info would need 'vendor' and 'model' field, These information is required for the drivermanger to select driver.. please check

it's not required here, they will be saved in Storage model

NajmudheenCT · 2020-05-05T11:22:25Z

dolphin/db/sqlalchemy/api.py

-        .filter(Storage.id == storage_id) \
-        .first()
-    return storage_by_id
+    return _storage_get_get(context, storage_id)


NajmudheenCT · 2020-05-05T11:24:09Z

dolphin/db/sqlalchemy/api.py

-    this_session.commit()
-    return register_ref
+    if not values.get('storage_id'):
+        values['storage_id'] = uuidutils.generate_uuid()


we discussed to move this to driver manager_ , if storage_id is not provided by driver, driver manager generates a uuid..

NajmudheenCT

LGTM

kumarashit · 2020-05-05T18:13:12Z

dolphin/api/v1/storages.py

+        access_info_dict = copy.deepcopy(access_info)
+
+        # Remove unrelated query fields
+        access_info_dict.pop('username', None)


How about:
del_list = {'username', 'password', 'vendor', 'model'}
list(map(access_info_dict.delitem, filter(access_info_dict.contains, del_list)))
or
import operator
map(lambda x: operator.delitem(access_info_dict, x), del_list)

kumarashit · 2020-05-05T18:18:18Z

dolphin/db/sqlalchemy/api.py

-    this_session.commit()
-    return register_ref
+    if not values.get('storage_id'):
+        values['storage_id'] = uuidutils.generate_uuid()


IMHO, this is ok to have double check but not a good practice. Anything related to storage attrib/ID should be handled till driver/driver manager layer and should not be exposed above. If the client doesn't get storage_id, it should throw exception and not try to create one.

kumarashit · 2020-05-05T18:19:56Z

dolphin/db/sqlalchemy/api.py

+    return _storage_get_get(context, storage_id)
+
+
+def _storage_get_get(context, storage_id, session=None):


What is this naming, storage_get_get? why can't it be simply get_storage or storage_get

kumarashit · 2020-05-05T18:25:14Z

dolphin/db/sqlalchemy/models.py

-    hostname = Column(String(128))
-    username = Column(String(128))
-    password = Column(String(128))
+    storage_id = Column(String(36), primary_key=True)


Trying to understand: Why is Storage_id the PK in access_info table? Should not this be FK or Composite? storage_id is ideally part of storage table

kumarashit

LGTM
[Some comments maybe addressed/revisited later as part of issue]

sfzeng

LGTM

Delete trap redundant attribute in 3Par

PravinRanjan10 reviewed May 1, 2020

View reviewed changes

NajmudheenCT requested changes May 4, 2020

View reviewed changes

NajmudheenCT reviewed May 4, 2020

View reviewed changes

sfzeng reviewed May 5, 2020

View reviewed changes

NajmudheenCT reviewed May 5, 2020

View reviewed changes

wisererik force-pushed the register-storage branch 2 times, most recently from 086b2e7 to bec2b6b Compare May 5, 2020 10:06

Add api of registering storage

dad4e0e

wisererik force-pushed the register-storage branch from bec2b6b to fe563fa Compare May 5, 2020 10:25

NajmudheenCT requested changes May 5, 2020

View reviewed changes

NajmudheenCT previously approved these changes May 5, 2020

View reviewed changes

kumarashit reviewed May 5, 2020

View reviewed changes

Address some comments and add exception handle

7cbac5e

wisererik dismissed NajmudheenCT’s stale review via 7cbac5e May 6, 2020 01:59

wisererik force-pushed the register-storage branch from fe563fa to 7cbac5e Compare May 6, 2020 01:59

kumarashit approved these changes May 6, 2020

View reviewed changes

sfzeng approved these changes May 6, 2020

View reviewed changes

kumarashit merged commit 852f452 into sodafoundation:master May 6, 2020

skdwriting pushed a commit that referenced this pull request Mar 22, 2021

Merge pull request #32 from yuanyu-ghca/hpe_3par0319

32e77f1

Delete trap redundant attribute in 3Par

		return _storage_get_get(context, storage_id)


		def _storage_get_get(context, storage_id, session=None):

Add api of registering storage #32

Add api of registering storage #32

Conversation

wisererik commented Apr 30, 2020

PravinRanjan10 May 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NajmudheenCT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NajmudheenCT May 5, 2020 • edited Loading

Choose a reason for hiding this comment

wisererik May 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NajmudheenCT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumarashit left a comment

Choose a reason for hiding this comment

sfzeng left a comment

Choose a reason for hiding this comment

PravinRanjan10 May 1, 2020 •

edited

Loading

NajmudheenCT May 5, 2020 •

edited

Loading

wisererik May 5, 2020 •

edited

Loading